Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add extra configuration options to gRPC #7672

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add extra configuration options to gRPC #7672

wants to merge 4 commits into from

Conversation

ErikDeSmedt
Copy link
Contributor

@ErikDeSmedt ErikDeSmedt commented Sep 16, 2024 •
edited
Loading

Pull Request Title

Description

I've added a few configuration options to the gRPC-plugin.

  • grpc-ip-address: I've changed the default ip-address to which the gRPC interface will bind from 0.0.0.0 to 127.0.0.1. This new default should be more secure. Users who need to access the gRPC-interface externally can set grpc-ip-address to the port they like. Users can also configure an IPv6 address.
  • grpc-scheme: Previously, the gRPC connection only allowed https over mTLS. Configurting mTLS is somewhat cumbersome. Users that only need the gRPC interface locally can now set grpc-scheme=http. This will only work if grpc-ip-address is set to a loopback address (127.0.0.1 or equivalent for IPv6).

Related Issues

Changes Made

  • [ x ] Feature: More config options for gRPC interface

Checklist

Ensure the following tasks are completed before submitting the PR:

  • Changelog has been added in relevant commit/s.
  • Tests have been added or updated to cover the changes.
  • Documentation has been updated as needed.
  • Any relevant comments or TODOs have been addressed or removed.

Additional Notes

Any additional information or context about this PR that reviewers should know.

@ErikDeSmedt ErikDeSmedt force-pushed the configure-grpc branch 4 times, most recently from 05e01e0 to 025bfc4 Compare September 17, 2024 18:42
@ErikDeSmedt
Copy link
Contributor Author

ErikDeSmedt commented Sep 20, 2024 •
edited
Loading

The test_grpc_connect_notification failed in the clang compilation target.

I tried to reproduce the failure locally.
To do this I compiled Core Lightning using clang.

CC=clang ./configure

Subsequently, I ran the test a 100 times.

for i={1..100}; do PYTEST_OPTS='-k test_cln_grpc_connect_notification' make pytest; done

I haven't observed a single failure. I'm not convinced the test is actually broken and don't have a way to reproduce

@rustyrussell
Copy link
Contributor

This needs a rebase, now that the grpc-host config option has been included.

@ShahanaFarooqui ?

@rustyrussell
Copy link
Contributor

This needs a rebase, since the grpc-host option is now merged (separately).

@ShahanaFarooqui
Copy link
Collaborator

This needs a rebase, since the grpc-host option is now merged (separately).

I did rebase it on grpc-host option but need to rebase again after 7807 merge. I also need to clean up some commits. Working on them right now.

@ShahanaFarooqui ShahanaFarooqui force-pushed the configure-grpc branch 2 times, most recently from 5b75526 to ec955e1 Compare November 13, 2024 04:04
@ErikDeSmedt @ShahanaFarooqui
grpc-plugin: Use default port and host + refactor
4f319e6 
A small refactor that allows to specify a more flexible configuration
in the grpc-plugin. By default the grpc-plugin will bind to 127.0.0.1
instead of 0.0.0.0.
@ErikDeSmedt @ShahanaFarooqui
grpc-plugin: Support grpc-scheme (http/https)
75396c8 
Also support using the cln-grpc plugin over http.
This is useful for testing. As a safe-guard we only
allow this configuration if the services binds to the loopback address.

Changelog-Added: `grpc-scheme` option for grpc plugin
@ErikDeSmedt @ShahanaFarooqui
doc: Update documentation for --grpc-scheme
11284d5
@ErikDeSmedt @ShahanaFarooqui
test: test grpc to run on http and to refuses non-loopback port on http
fdf3cf3
@ErikDeSmedt
Copy link
Contributor Author

Some of the functionality overlaps #7479. I was not aware at the time of writing.

Both this PR and #7479 introduce a default on the host and port.
This PR also introduced the option to run the grpc-interface on http if the loopback address is used

I believe it is sensible to close this PR and tackle the http-interface in a new PR.

@ShahanaFarooqui
Copy link
Collaborator

I believe it is sensible to close this PR and tackle the http-interface in a new PR.

@ErikDeSmedt I have rebased this PR onto master, which now includes the merged PR #7479 (default grpc-port and grpc-host). I adjusted your commits to add support for grpc-scheme and restrict HTTP access to the loopback address only. However, since I am still getting familiar with Rust, I would really appreciate it if you could review these changes.

@ErikDeSmedt
Copy link
Contributor Author

ACK fdf3cf3

ErikDeSmedt
ErikDeSmedt commented Nov 15, 2024
View reviewed changes
Copy link
Contributor Author

@ErikDeSmedt ErikDeSmedt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebase looks great

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdecker cdecker Awaiting requested review from cdecker cdecker is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cln-grpc: Specify a host and port
3 participants
@ErikDeSmedt @rustyrussell @ShahanaFarooqui